Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: default enable allowDynamicBackends with better unsupported errors #995

Merged
merged 10 commits into from
Oct 15, 2024

Conversation

guybedford
Copy link
Member

@guybedford guybedford commented Oct 3, 2024

This changes the default in the JS SDK to remove the secondary allowDynamicBackends(true) call which is currently required in order to use dynamic backends, even when they are enabled at the service level.

Intead, with this PR, calling new Backend(...) will automatically try to create a dynamic backend, returning a better error message when not enabled:

Backend constructor: Unable to create a dynamic backend for 'http-me.glitch.me' - dynamic backends are unsupported on this service. Either explicitly configure backend services or contact Fastly support to enable dynamic backends.

Then secondly, when calling an arbitrary fetch() request without providing an explicit backend option, it will also try and create a dynamic backend automatically instead returning this new error message:

fetch(): No backend provided fetching 'https://http-me.glitch.me/something'. Since dynamic backends are not enabled for this Fastly service, fetch() requires an explicit backend parameter. See https://js-compute-reference-docs.edgecompute.app/docs/globals/fetch for more info. Alternatively, contact Fastly support to enable dynamic backends.

Then to provide the previous security property of requiring explicit backends, we have a new import { enforceExplicitBackends } from 'fastly:backend' which can be used when users want to lock down their backend usage.

We cannot override the service-level configuration from the JS SDK, so the default of them not being enabled remains, but we can make the default experience better without dynamic backends and remove confusion as to why it has to be "enabled twice" currently.

In addition, this PR also adds a new Backend.prototype.name readonly getter and deprecates backend.toName() to use backend.name instead.

Resolves 987.

@guybedford guybedford force-pushed the default-enable-dynamic-backends branch 2 times, most recently from 9e35abd to 8f79e16 Compare October 3, 2024 17:41
@guybedford guybedford force-pushed the default-enable-dynamic-backends branch from a69a2ca to 5b0712e Compare October 3, 2024 19:14
@guybedford
Copy link
Member Author

guybedford commented Oct 3, 2024

Two further suggestions that might make sense still here:

  1. In the error for fetch() to explicitly call out that Fastly accepts a backend parameter, so making the error message more specific to this case.
  2. Changing the imported function on fastly:backend to import { enforceExplicitBackends } from 'fastly:backend' or similar, to more clearly demonstrate this is now a security feature that can be used.

@guybedford
Copy link
Member Author

I've added the suggestion here in renaming to enforceExplicitBackends with an optional default backend argument, and updating the error messages and types.

@guybedford guybedford changed the title feat: default enable allowDynamicBackends, better unsupported error feat: default enable allowDynamicBackends with better unsupported errors Oct 5, 2024
@chrisfarber
Copy link

I especially like being able to set the implicit backend for any fetch() call via enforceExplicitBackends(), as this gives a bit more control over your third party dependencies.

Given the security and configuration benefits of using static backends, and the fact that you can't see on the manage UI whether dynamic backends are enabled for a service, I'm a bit conflicted over whether to enable them by default. Our team began using dynamic backends, but has since switched our JS code to exclusively use static backends — despite this, IIRC, dynamic backends were left enabled on the services. This is a good reminder for us to fix that, but, for customers in our situation, this probably represents a breaking change worth noting.

I had an idea while reviewing this; what if:

  • by default, dynamic backends will be allowed only if you pass a special flag to fetch, roughly something like fetch("http://fastly.com", { dynamicBackend: true }). This would let you use them intentionally in your own code, while providing some safety from third party dependencies.
  • You could then call a function like setImplicitBackend({ backend: ... }), useful for any third party libraries making fetch() calls, or just to reduce boilerplate wherever the compute service calls fetch()
  • Or globally allow implicit dynamic backends like setImplicitBackend({ dynamicBackend: true }), to opt out of any protection

This would still let you use dynamic backends without enabling them twice, and provide some safety/control of third party dependencies, with the ability to opt out for convenience. But obviously it's a larger breaking change.

@guybedford
Copy link
Member Author

Thanks for the thoughtful points here, it's really appreciated in trying to consider what API is best for users. I think the benefit of dynamic backends is that they allow code to run without modification from other providers, which may be useful for new users.

It is perfectly valid though for us to conclude here that allowDynamicBackends(true) remains the recommended process, this PR was just about investigating if we want to prioritise support standards-compatibility in the name of being user friendly, versus security.

But since the security is already handled on the service-level, flipping the default seemed like it didn't have to tradeoff between both.

On the other hand, as you say, being able to set the default implicit backend may be enough in many cases too.

I'm not going to rush landing this as I think it requires some careful though, but hopefully we can work towards the right user experience going forward.

@guybedford
Copy link
Member Author

Putting some more thought here - the Go SDK doesn't have a separate runtime-level flag itself, and neither does the Rust SDK, therefor I would like to move ahead with landing this.

@guybedford
Copy link
Member Author

guybedford commented Oct 11, 2024

This PR is now ready to merge, I will land and release on Monday unless there is any further feedback.

@guybedford guybedford merged commit bb858fe into main Oct 15, 2024
25 checks passed
@guybedford guybedford deleted the default-enable-dynamic-backends branch October 15, 2024 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default to allowDynamicBackends
3 participants